BTree: no longer copy keys and values before dropping them #84904
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
Various versions show the same picture, which is generally positive but only for fat values, and rarely as crazy as |
jyn514
added
I-slow
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit bfe6ccf876ccead4d8cd95cd435ebfb523bd6410 with merge 5eef46766aae4d1e3fc87be64eb4ff53d556a5bb... |
|
Looks good to me overall, presuming perf comes back clean. |
|
☀️ Try build successful - checks-actions |
|
Queued 5eef46766aae4d1e3fc87be64eb4ff53d556a5bb with parent 109248a, future comparison URL. |
|
Finished benchmarking try commit (5eef46766aae4d1e3fc87be64eb4ff53d556a5bb): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
It looks like this is around neutral on instruction counts, but we do see a pretty large regression in wall times on the await-call-tree benchmarks, which likely is worth investigating. Other benchmarks are seeing pretty excellent improvements though in wall times, though. That said something is really odd about these results- I'm seeing no corresponding effects to cycles or cpu-clock. Maybe something happened in the benchmarking environment, though I don't know what it could be. If you can push a rebased commit we can re-run perf here and see if we get similar results, I'm not sure that I trust these. |
ssomers
force-pushed
the
btree_drop_kv_in_place
branch
from
bfe6ccf
to
728204b
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 728204b with merge 34497e6e26bdd3159376c15a31e14950a8784a08... |
|
☀️ Try build successful - checks-actions |
|
Queued 34497e6e26bdd3159376c15a31e14950a8784a08 with parent 6fd7a6d, future comparison URL. |
|
Finished benchmarking try commit (34497e6e26bdd3159376c15a31e14950a8784a08): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
Indeed appears to have been a random blip, not sure what caused it. Thanks! @bors r+ |
|
📌 Commit 728204b has been approved by |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit 728204b with merge 68ce31e0d7e65111932ba1285aead9af394b7bc2... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Looks like there's an identical x86_64-msvc-1 and x86_64-msvc-2 which failed and succeeded respectively. Another case of #83262? |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
compiler-errors
added a commit
to compiler-errors/rust
that referenced
this pull request
Nov 22, 2024
btree: don't leak value if destructor of key panics This PR fixes a regression from rust-lang#84904. The `BTreeMap` already attempts to handle panicking destructors of the key-value pairs by continuing to execute the remaining destructors after one destructor panicked. However, after rust-lang#84904 the destructor of a value in a key-value pair gets skipped if the destructor of the key panics, only continuing with the next key-value pair. This PR reverts to the behavior before rust-lang#84904 to also drop the corresponding value if the destructor of a key panics. This avoids potential memory leaks and can fix the soundness of programs that rely on the destructors being executed (even though this should not be relied upon, because the std collections currently do not guarantee that the remaining elements are dropped after a panic in a destructor). cc `@Amanieu` because you had opinions on panicking destructors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 24, 2024
btree: don't leak value if destructor of key panics This PR fixes a regression from rust-lang#84904. The `BTreeMap` already attempts to handle panicking destructors of the key-value pairs by continuing to execute the remaining destructors after one destructor panicked. However, after rust-lang#84904 the destructor of a value in a key-value pair gets skipped if the destructor of the key panics, only continuing with the next key-value pair. This PR reverts to the behavior before rust-lang#84904 to also drop the corresponding value if the destructor of a key panics. This avoids potential memory leaks and can fix the soundness of programs that rely on the destructors being executed (even though this should not be relied upon, because the std collections currently do not guarantee that the remaining elements are dropped after a panic in a destructor). cc `@Amanieu` because you had opinions on panicking destructors
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When dropping BTreeMap or BTreeSet instances, keys-value pairs are up to now each copied and then dropped, at least according to source code. This is because the code for dropping and for iterators is shared.
This PR postpones the treatment of doomed key-value pairs from the intermediate functions
deallocating_next(_back) to the last minute, so the we can drop the keys and values in place. According to the library/alloc benchmarks, this does make a difference, (and a positive difference with an#[inline]ondrop_key_val). It does not change anything for #81444 though.r? @Mark-Simulacrum